Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A bunch of assorted patches related to socket activation feature (part 1) #6873

Closed
wants to merge 10 commits into from

Conversation

alexey-tikhonov
Copy link
Member

No description provided.

Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thanks for the patches, I'm fine with all the changes, ACK.

bye,
Sumit

Copy link
Contributor

@justin-stephenson justin-stephenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, thank you.

Copy link
Member

@pbrezina pbrezina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in SYSTEMD: removed unneeded capabilities commit message in CAP_SET?ID. Otherwise ack.

@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Sep 4, 2023

Typo in SYSTEMD: removed unneeded capabilities commit message in CAP_SET?ID. Otherwise ack.

I meant to denote both CAP_SETUID and CAP_SETGID. Ok, I'll change it.

Code makes no difference handling '--socket-activated' and
'--dbus-activated', it only makes things more obscure.
Moreover, on a systemd enabled system, dbus activation actually
starts systemd service anyway, so there is really no big difference.
since implicit files provider can't be enabled by default anymore.

Resolves: SSSD#5022
This patch removes capabilities that aren't needed at all.

Some (if not all) of remaining capabilities can be probably
avoided with proper code changes, but currently those are needed.

Examples (not limiting) of those caps usage:
 - CAP_DAC_OVERRIDE (@additional_caps@): access to /var/log/sssd,
   to /var/lib/sss/pipes/private/* (sssd:sssd owned sbus-monitor/dp
   sbus sockets)
 - CAP_CHOWN: `chown_debug_file()` in case of monitor activation
 - CAP_SETUID/CAP_SETGID: drop privs in case of monitor activation,
   switch_creds (in particular, sssd_kcm executing krb5_child
   for ticket renewal)
 - CAP_FOWNER: chmod(mem-cache)

It's not that clear about 'CAP_KILL'. When 'sssd_be' terminates
child process, it either still runs under root (so uid matches and
no caps needed) or it dropped privs already and have lost CAP_KILL
anyway. Another thing is 'monitor' signalling responders and
providers that could be running under 'sssd' while 'monitor'
itself runs under 'root'.
This allows to remove CAP_FOWNER.
'PermissionsStartOnly' is deprecated but used for consistency
with other unit files.
@alexey-tikhonov
Copy link
Member Author

Typo in SYSTEMD: removed unneeded capabilities commit message in CAP_SET?ID. Otherwise ack.

I meant to denote both CAP_SETUID and CAP_SETGID. Ok, I'll change it.

Done.

@pbrezina
Copy link
Member

pbrezina commented Sep 5, 2023

Typo in SYSTEMD: removed unneeded capabilities commit message in CAP_SET?ID. Otherwise ack.

I meant to denote both CAP_SETUID and CAP_SETGID. Ok, I'll change it.

Ah, I did not get that :-)

@alexey-tikhonov
Copy link
Member Author

Pushed PR: #6873

  • master
    • 9d7dd81 - SYSTEMD: several comments to service files
    • 8e1d2bb - SYSTEMD: replace deprecated 'PermissionsStartOnly=true' with '+'
    • 9cb3972 - SYSTEMD::IFP: don't restrict ExecStartPre=chown(log)
    • 19c741c - SYSV/NSS: avoid chmod() in sssd_nss
    • 49f59cd - SYSTEMD: removed unneeded capabilities
    • abd9130 - MONITOR: debug messages updates
    • e0903de - SBUS: additional details in debug messages
    • b639f33 - CONF: there is no use for CONFDB_FALLBACK_CONFIG
    • 50e7891 - CONFDB: removed unneeded wrapper
    • c4b5fda - Get rid of '--dbus-activated'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport This should go to target branch only. non-privileged Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants